relations: Add functionality to sort json refs by relation_type #1202
Conversation
sakshamarora1
force-pushed
the
sort_relations_in_json_resolvers
branch
from
42b6d58
to
2ae5c56
Compare
| @@ -26,7 +26,7 @@ def jsonresolver_loader(url_map): | |||
| def relations_resolver(document_pid): | |||
| """Resolve record relations and add metadata.""" | |||
| document = Document.get_record_by_pid(document_pid) | |||
| return document.relations | |||
| return document.relations(sort_by = ["edition"]) | |||
There was a problem hiding this comment.
Question: I guess that sort_by is a list because we want to be able to define a different sort type for the different type of relations?
There was a problem hiding this comment.
Yes, as relations have a separate object list in the response
| @@ -170,6 +170,11 @@ def get(self): | |||
| r = self._build_relation_obj(sibling_pid, name) | |||
| relations.setdefault(name, []) | |||
| relations[name].append(r) | |||
|
|
|||
| # Pre-requisite: fields being sorted should be of the same type. | |||
| if sort_by and name in sort_by and name in relations.keys(): | |||
There was a problem hiding this comment.
Question: Does this mean that the EDITION_RELATION can only be sorted by edition, LANGUAGE_RELATION only by language and OTHER_RELATION by other?
There was a problem hiding this comment.
Yes!
Do we want the ability to sort by another field apart from the relation type?
| @@ -421,12 +421,10 @@ def remove(self, previous_rec, next_rec, relation_type): | |||
| class IlsRecordWithRelations(IlsRecord): | |||
| """Add relations functionalities to records.""" | |||
|
|
|||
| @property | |||
| def relations(self): | |||
| def relations(self, sort_by=None): | |||
There was a problem hiding this comment.
this is a breaking change in the class interface, did you test all the possible occurences?
There was a problem hiding this comment.
in addition: I think the sort should be somewhat defined per relation type (maybe config of relation types?) not as an argument of the function - we risk the list of arguments growing infinitely
| @@ -421,12 +421,10 @@ def remove(self, previous_rec, next_rec, relation_type): | |||
| class IlsRecordWithRelations(IlsRecord): | |||
| """Add relations functionalities to records.""" | |||
|
|
|||
| @property | |||
| def relations(self): | |||
| def relations(self, sort_by=None): | |||
There was a problem hiding this comment.
we could go for kwargs instead:
| def relations(self, sort_by=None): | |
| def relations(self, **kwargs): |
| @@ -224,14 +229,14 @@ def get(self): | |||
| return relations | |||
|
|
|||
|
|
|||
| def get_relations(record): | |||
| def get_relations(record, sort_by=None): | |||
There was a problem hiding this comment.
| def get_relations(record, sort_by=None): | |
| def get_relations(record, **kwargs): |
and also inject kwargs in all the .get() methods
|
|
||
| # Pre-requisite: fields being sorted should be of the same type. | ||
| if sort_by and name in sort_by and name in relations.keys(): | ||
| relations[name].sort(key=lambda rec: rec["record_metadata"][name]) |
There was a problem hiding this comment.
This might not good enough: we might be sorting too late, sorting the results of what was retrieved from the db, queried with a default sorting.
If this is what we want to do, then it might better to have the client, consuming the REST API response, to sort the results.
If instead we want to drive the sorting from the backend, then the sorting param should be applied to the db query.
However, this is very tricky, due to the nature of data stored as JSON blob.
There was a problem hiding this comment.
Discussed IRL, decision to be taken.
invenio_app_ils/relations/api.py
Outdated
| @@ -27,6 +27,7 @@ | |||
| "invenio_app_ils.relations.nodes:PIDNodeRelated", | |||
| "invenio_pidrelations.serializers.schemas.RelationSchema", | |||
| "invenio_app_ils.relations.api.SiblingsRelation", | |||
| [], | |||
There was a problem hiding this comment.
| [], | |
| None, |
Setting mutable parameters is in general considered as bad practice, since we can change it on a runtime - especially in this case it is configuration so we don't want to allow for changes
❤️ Thank you for your contribution!
Description
Closes: CERNDocumentServer/cds-ils#827